test(a3p): test mintHolder functionalities after contract null upgrade #10617

Jorge-Lopes · 2024-12-04T15:58:47Z

Description

This pull request intends to ensure that the mintHolder contract continues to function as expected after a null upgrade.

Key Changes:

A new core-eval was built, which acordingly to the agoric chain variant, will select a list of vbankAsset and execute a null upgrade to the respective mintHolder contract of each suitable asset.
A new acceptance test, mintHolder.test.js, has been added to the a3p-integration to verify the core-eval behaviour.

MintHolder Test Plan:

Provision a receiver wallet with the respective assets
Confirm the initial balance matches expectations
Perform a null upgrade for the mintHolder contracts
Execute a core-eval to:
- Mint a payment for each selected asset
- Deposit the payment into the receiver's depositFacet
Verify that the balance has increased by the expected amount
Execute a PSM swap for selected assets with IST

Security Considerations

The execution of mintHolder.test.js had to be done prior to the ./genesis-test.sh acceptance test. Otherwise it would trigger the error bundle ${id} not loaded during the evalBundle call.
See this issue for more detail: #10620

Scaling Considerations

This PR does not introduce any change to the mintHolder source code, or how it is being consumed. For this reason that are no scaling considerations to be made.

Documentation Considerations

Testing Considerations

The implemented test design is expecting to have a core-eval for each mintHolder being updated.
If desired, this could be refactored to have a single core-eval executing a null upgrade to a predefined list of assets.
The second approach has the advantage of reducing the number of files being created, although it may hinder the testing flexibility.

Which approach is more desirable?

Upgrade Considerations

cloudflare-workers-and-pages · 2024-12-04T15:59:02Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`aa864ce`
Status:	✅ Deploy successful!
Preview URL:	https://5993b108.agoric-sdk.pages.dev
Branch Preview URL:	https://jorge-10410-mint-holders-upg.agoric-sdk.pages.dev

View logs

Chris-Hibbert · 2024-12-04T22:44:59Z

When choosing assets to start with in upgrading MintHolders on mainNet, I'd like to start with a few that don't have the highest values at stake. When I look at the interProtocol dashboard (https://info.inter.trade/psm?chain=mainnet), I see that USDC and DAI_axelar have the highest amounts. I don't see a dashboard that lists BLD, but I'd expect it to be very high. For vaults, I see stOSMO, stATOM, and stTIA having the highest collateral values.

The mintHolder vats on MainNet include BLD, ATOM, USDC_axl, USDC_grv, USDT_axl, USDT_grv, DAI_axl, DAI_grv, stATOM, USDC, USDT, stOSMO, stTIA, and stkATOM. Rather than starting with BLD, ATOM and USDC, on MainNet, let's start with ATOM, USDC_axl, USDC_grv, USDT_axl, USDT, and stkATOM.

I'd like the test to trade some of the currency with a PSM or Vault. (Whichever is easier; it's not valuable to do both.) Oh, I don't want this PR to have to create PSMs or add vault types, so it makes more sense to use the vaults and PSMs that exist in a3p-integration for the test. That means the list of mintHolders to be upgraded should be parameterized, so we can upgrade different combinations in a3p, on our testnets, and when we go to MainNet.

If desired, this could be refactored to have a single core-eval executing a null upgrade to a predefined list of assets.
The second approach has the advantage of reducing the number of files being created, although it may hinder the testing flexibility.

Yeah, it should be a single script that takes a parameter to say which vat/currency.

a3p-integration/proposals/z:acceptance/mint-test-submission/send-script.tjs

Chris-Hibbert · 2024-12-04T23:22:58Z

packages/vats/src/proposals/upgrade-mintHolder-atom-proposal.js

+  ]);
+
+  const mintHolderKit = Array.from(contractKits.values()).filter(
+    kit => kit.label && kit.label.match(/ATOM/),


lots of vats have ATOM in their names. Let's find a better way to uniquely identify the mintHolder vat.

The name also has 'mintHolder' in it, so that's not unique yet. On MainNet, there are vats for ATOM, stATOM, and stkATOM as well as two different kinds of USDC, USDC, and DAI. should we specify the vat number, which is unique (though different) on each platform?

I think I'll post something to the kernel team. They often have opinions and ideas about this kind of thing.

I think everyone would be satisfied if we first filter for the installation, and then look for the exact name. At the moment, there's only one installation for mintHolder, so we can just get the unique mintHolder installation from the promise space.

Please clarify if you meant instance instead of installation?
I am asking because, if I am not mistaken, the contractKits only hold the contract instance, adminFacet, creatorFacet and publicFacet.

And if that is the case, from where can we retrieve the mintHolder Instance in order to compare it with the ones recorded in the contractKits?

I did mean installation, but you're right, that's not available. I was happy with that because one installation is shared across many instances. The instances aren't helpful for this.

The vat names are unique, but the labels used in contractKits are just the issuer name.

Dan had suggested using __getMethodNames__() on the publicFacet to distinguish mintHolders. The publicFacet of a mintHolder is the issuer itself. (I didn't realize you could call that on a remote reference, but Dan thinks so.) If you use an exact match on the label, and verify that the publicFacet has a makeEmptyPurse method, then I'll be comfortable that you've found the right contract and it's a mintHolder. Does that work? Is the issuer name used for the label sufficient to distinguish the varieties of USDT and DAI?

The tests showed that using strict equality operator to compare the label with the issuer name, is indeed capable of distinguish the varieties of USDT, USDC and DAI assets.

I was not able to find an example of getMethodNames() being used on Agoric code, and I while trying different implementations for it, I was not successful to find one that would return a list of exposed methods by the mintHolder publicFacet.

Although, at commit b9c5acb, I followed the approach you suggested by calling the makeEmptyPurse method and verify that the core-eval did not throw during the test execution.

Is this sufficient to confirm that we are interacting with the expected contract?

Jorge-Lopes · 2024-12-05T17:21:36Z

I'd like the test to trade some of the currency with a PSM or Vault.

List of anchor assets available for PSM at a3p-integration:

DAI_axl
DAI_grv
USDC
USDC_axl
USDC_grv
USDT_axl
USDT_grv

List of collateral of available open Vaults a3p-integration:

ATOM
stATOM

Based on the list above, it makes more sense to extend the mintHolder tests coverage by executing a swap via PSM.
@Chris-Hibbert Should we test all the assets displayed on the PSM list or only the ones that overlap with the list you provided?

ATOM, USDC_axl, USDC_grv, USDT_axl, USDT, and stkATOM.

Meaning: USDC_axl, USDC_grv, USDT_axl

Chris-Hibbert · 2024-12-05T18:48:14Z

Based on the list above, it makes more sense to extend the mintHolder tests coverage by executing a swap via PSM.
@Chris-Hibbert Should we test all the assets displayed on the PSM list or only the ones that overlap with the list you provided?

If we have a simple mechanism to describe which PSMs should be upgraded in which environment, there's no reason not to do them all in a3p.

Jorge-Lopes · 2024-12-05T20:45:16Z

Following the suggested design, I updated the mintHolder acceptance test to fetch the Vstorage children nodes of published.psm.IST , this way we can get the list of available assets for each desired network config.

Then it will iterate through this list, and for each asset, it constructs an object containing the following attributes:

Label
MintHolder Vat Name
Denom (retrieved from the agoricNames.vbankAssets)

The mintHolder contract upgrade and functionality tests are then executed for each asset included in the list.

Currently, the assets DAI_axl, DAI_grv, and USDC are excluded due to the following issues:

DAI_axl and DAI_grv assets exhibit abnormal behavior during PSM trade execution.
For USDC when passing zcf-mintHolder-USDC as the vatName argument to the getIncarnation method, it returns the incarnation of zcf-mintHolder-USDC_axl instead.

I am actively working to resolve these issues, as well as updating the core-eval proposal to rely on the installation rather than only the label.

@Chris-Hibbert do you agree with the test design currently implemented?

Chris-Hibbert · 2024-12-05T21:31:18Z

do you agree with the test design currently implemented?

Yep, seems fine.

Jorge-Lopes · 2024-12-09T18:20:37Z

During the execution of a PSM swap for DAI variants, an issue was encountered. Since this issue is outside the scope of the current pull request, I have opened a new issue (#10655) to address it separately.

In mintHolder.test.js, I added a condition to skip the PSM swap when the asset label includes DAI. However, the test still verifies that the mintHolder contract is upgraded and that the payment is minted as expected.

Additionally, I included an inline comment linking to the new issue and indicating that the condition should be removed once the issue is resolved.

@Chris-Hibbert, does this approach allow us to proceed with landing this pull request?

Chris-Hibbert

The purpose of #10410 is to upgrade the mintHolder vats on mainNet. This PR seems to focus on testing. Will it include adding invocations to upgrade.go?

This PR also only upgrades mintHolders for currencies associated with PSMs. What about BLD, stATOM, stkATOM, stTIA, and stOSMO?

I was hoping that we'd be able to set up upgrade.go, so it could invoke upgrade mintHolders with a list of labels or assets, rather than creating a separate proposal for each currency to be upgraded. If we can do that, then the test should also invoke a single proposal and pass in a specification of which vats/currencies to handle rather than creating separate proposals in package.json.

a3p-integration/proposals/z:acceptance/mint-test-submission/send-script.tjs

a3p-integration/proposals/z:acceptance/mintHolder.test.js

Chris-Hibbert · 2024-12-09T22:16:01Z

packages/vats/src/proposals/upgrade-mintHolder-proposal.js

+   * Ensure that publicFacet holds an issuer by calling makeEmptyPurse, the
+   * core-eval will throw if otherwise.
+   */
+  await E(publicFacet).makeEmptyPurse();


Can we verify that the method is present without calling it? try __getMethodNames__().

I was not able to find an example of getMethodNames() being used on Agoric code, and while trying your suggestion it triggers the following error:

target has no method __getMethodNames__ , has [ 'burn', 'getAllegedName', 'getAmountOf', 'getAssetKind', 'getBrand', 'getDisplayInfo', 'isLive', 'makeEmptyPurse' ]

For that reason, I am invoking the makeEmptyPurse method itself

It looks like you probably tried E(publicFacet). __getMethodNames__(). I think what we want is publicFacet. __getMethodNames__(). (I realize this is surprising.) I've asked @dckc to confirm.

When trying the implementation you suggested, the error function not found is triggered.
I intend to address this subject on the office hours.

calling makeEmptyPurse() seems better than bothering with __getMethodNames__().

Jorge-Lopes · 2024-12-10T23:20:59Z

I was hoping that we'd be able to set up upgrade.go, so it could invoke upgrade mintHolders with a list of labels or assets, rather than creating a separate proposal for each currency to be upgraded.

Regarding the mintHolder upgrade core-eval, the implementation has been enhanced to use the endowments.scriptArgs for passing a list of assets to be updated. This allows all mintHolder vats to be upgraded in a single proposal, eliminating the need for separate proposals per Label.

This PR also only upgrades mintHolders for currencies associated with PSMs. What about BLD, stATOM, stkATOM, stTIA, and stOSMO?

... If we can do that, then the test should also invoke a single proposal and pass in a specification of which vats/currencies to handle rather than creating separate proposals in package.json.

I have updated the test design in order to execute a single submission and verify that, according to the network configuration provided, all bankAssets listed on Vstorage that has a respective mintHolder Vat were successfully upgraded.

For all the assets identified in the step above, the mintHolder test will mint a payment and verify the wallet balance.
Following this process, the list of available PSM anchor assets will be fetch from Vstorage and a swap with IST is executed.

@Chris-Hibbert I hope that this new design provides the testing coverage intended.

The purpose of #10410 is to upgrade the mintHolder vats on mainNet. This PR seems to focus on testing. Will it include adding invocations to upgrade.go?

Yes, it is planned to add the upgrade-mintHolder.js to upgrade.go, although, due to the fact that it receives a list of assets as argument, I am trying to better understand the behaviour of buildProposalStepWithArgs and how to test its execution.

Chris-Hibbert · 2024-12-11T00:44:07Z

a3p-integration/proposals/p:upgrade-19/package.json

@@ -10,13 +10,14 @@
      "testing/test-upgraded-board.js testUpgradedBoard",
      "vats/upgrade-agoricNames.js agoricNamesCoreEvals/upgradeAgoricNames",
      "testing/add-USD-OLIVES.js agoricNamesCoreEvals/addUsdOlives",
-      "testing/publish-test-info.js agoricNamesCoreEvals/publishTestInfo"
+      "testing/publish-test-info.js agoricNamesCoreEvals/publishTestInfo",
+      "vats/upgrade-mintHolder.js upgrade-mintHolder USDC_axl USDT_grv DAI_axl DAI_grv stATOM USDC_grv ATOM USDT_axl USDC BLD IST"


There's no mintholder for IST. What can we add to alert us when one of the requested upgrades isn't possible?

Initially, I used an assertion on the core-eval to verify that a mintHolder contract kit existed for each asset label. However, because IST was the last item in the list, the test passed even when a mintHolder was missing.

To ensure that the core-eval process doesn't fail completely while still identifying issues, I added a condition (see here) . If the condition fails, an error is logged specifying the asset that couldn't be upgraded. However, the process continues without throwing an exception.

Chris-Hibbert · 2024-12-11T00:47:35Z

packages/vats/src/proposals/upgrade-mintHolder-proposal.js

+   * Ensure that publicFacet holds an issuer by calling makeEmptyPurse, the
+   * core-eval will throw if otherwise.
+   */
+  await E(publicFacet).makeEmptyPurse();


It looks like you probably tried E(publicFacet). __getMethodNames__(). I think what we want is publicFacet. __getMethodNames__(). (I realize this is surprising.) I've asked @dckc to confirm.

Jorge-Lopes · 2024-12-11T15:07:06Z

The upgrade-mintHolder.js core-eval has been redesigned to align with the approach used in updatePriceFeeds.js. Instead of requiring the entire asset label list to be passed as an argument, it now only takes the chain variant as input. Based on this variant, the mintHolder core-eval automatically provides the corresponding asset list to the proposal.

The asset lists used in upgrade-mintHolder.js for each chain variant were retrieved from the vbankAsset node using the Vstorage Viewer.

The upgrade.go file was updated accordingly.

Chris-Hibbert · 2024-12-12T00:25:37Z

a3p-integration/proposals/p:upgrade-19/mintHolder.test.js

+const networkConfig = {
+  rpcAddrs: ['http://0.0.0.0:26657'],
+  chainName: 'agoriclocal',
+};


Shouldn't this be a default for getPSMChildren? If this usage isn't doing anything unusual, it shouldn't have to specify this value.

Change request addressed

Chris-Hibbert · 2024-12-12T01:13:09Z

a3p-integration/proposals/p:upgrade-19/test-lib/psm-helpers.js

+  const [settlement] = await Promise.allSettled([
+    execa('agd', args, { all: true }),
+  ]);
+


I don't think this is different from

Suggested change

const [settlement] = await Promise.allSettled([

execa('agd', args, { all: true }),

]);

const settlement = await execa('agd', args, { all: true }));

After reviewing this change request, I realized that following the rebase to master, the file psm-lib.js had already been included in the upgrade-19 proposal's test library. To avoid duplicating code, I removed psm-helpers.js and updated psm-lib.js with the missing functions.

Regarding the implementation of sendOfferAgd, it was copied from z:acceptance/test-lib/psm-lib.js. The use of Promise.allSettled() was introduced in this commit by Richard Gibson.

While I am unsure of the specific rationale behind this design, the change you suggested triggers an error because the expected structure of the settlement object is not provided.

For these reasons, I plan to leave psm-lib.js as is.

It's different: await throws if the promise rejects. allSettled does not.

sendOfferAgd seems to report a rejection as a return value rather than throwing. Odd, but presumably expedient.

Jorge-Lopes added the force:integration Force integration tests to run on PR label Dec 4, 2024

Jorge-Lopes force-pushed the jorge/10410-mint-holders-upgrade branch from fb05f20 to 9e49ac7 Compare December 4, 2024 19:45

Jorge-Lopes requested review from Chris-Hibbert and iomekam December 4, 2024 21:03

Jorge-Lopes marked this pull request as ready for review December 4, 2024 21:03

Jorge-Lopes requested a review from a team as a code owner December 4, 2024 21:03

Chris-Hibbert reviewed Dec 4, 2024

View reviewed changes

Chris-Hibbert requested changes Dec 9, 2024

View reviewed changes

Jorge-Lopes force-pushed the jorge/10410-mint-holders-upgrade branch 2 times, most recently from b97559d to 4468330 Compare December 10, 2024 22:35

Chris-Hibbert reviewed Dec 11, 2024

View reviewed changes

Jorge-Lopes force-pushed the jorge/10410-mint-holders-upgrade branch from 82fe7d1 to 4c35d6e Compare December 11, 2024 14:44

Jorge-Lopes requested a review from Chris-Hibbert December 11, 2024 16:31

Chris-Hibbert approved these changes Dec 12, 2024

View reviewed changes

Jorge-Lopes added the automerge:rebase Automatically rebase updates, then merge label Dec 12, 2024

Jorge-Lopes added 4 commits December 12, 2024 12:16

test(a3p): add mintHolder upgrade test and helpers

01a15d0

chore(a3p): add submission and update client-utils version

02fde3c

chore(mintHolder): add mintHolder null upgrade core-eval

6ef9705

fix(a3p): remove usage of duplicated code

aa864ce

Jorge-Lopes force-pushed the jorge/10410-mint-holders-upgrade branch from e960919 to aa864ce Compare December 12, 2024 12:16

mergify bot merged commit 059601c into master Dec 12, 2024
82 checks passed

mergify bot deleted the jorge/10410-mint-holders-upgrade branch December 12, 2024 13:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(a3p): test mintHolder functionalities after contract null upgrade #10617

test(a3p): test mintHolder functionalities after contract null upgrade #10617

Jorge-Lopes commented Dec 4, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Dec 4, 2024 •

edited

Loading

Chris-Hibbert commented Dec 4, 2024

Chris-Hibbert Dec 4, 2024

Chris-Hibbert Dec 5, 2024

Jorge-Lopes Dec 6, 2024 •

edited

Loading

Chris-Hibbert Dec 6, 2024 •

edited

Loading

Jorge-Lopes Dec 9, 2024

Jorge-Lopes commented Dec 5, 2024 •

edited

Loading

Chris-Hibbert commented Dec 5, 2024

Jorge-Lopes commented Dec 5, 2024 •

edited

Loading

Chris-Hibbert commented Dec 5, 2024 •

edited

Loading

Jorge-Lopes commented Dec 9, 2024

Chris-Hibbert left a comment

Chris-Hibbert Dec 9, 2024

Jorge-Lopes Dec 10, 2024

Chris-Hibbert Dec 11, 2024

Jorge-Lopes Dec 11, 2024

dckc Dec 11, 2024

Jorge-Lopes commented Dec 10, 2024 •

edited

Loading

Chris-Hibbert Dec 11, 2024

Jorge-Lopes Dec 11, 2024 •

edited

Loading

Chris-Hibbert Dec 11, 2024

Jorge-Lopes commented Dec 11, 2024

Chris-Hibbert Dec 12, 2024

Jorge-Lopes Dec 12, 2024

Chris-Hibbert Dec 12, 2024

Jorge-Lopes Dec 12, 2024

dckc Dec 13, 2024

test(a3p): test mintHolder functionalities after contract null upgrade #10617

test(a3p): test mintHolder functionalities after contract null upgrade #10617

Conversation

Jorge-Lopes commented Dec 4, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Dec 4, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

Chris-Hibbert commented Dec 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jorge-Lopes Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Chris-Hibbert Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jorge-Lopes commented Dec 5, 2024 • edited Loading

Chris-Hibbert commented Dec 5, 2024

Jorge-Lopes commented Dec 5, 2024 • edited Loading

Chris-Hibbert commented Dec 5, 2024 • edited Loading

Jorge-Lopes commented Dec 9, 2024

Chris-Hibbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jorge-Lopes commented Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Jorge-Lopes Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jorge-Lopes commented Dec 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jorge-Lopes commented Dec 4, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Dec 4, 2024 •

edited

Loading

Jorge-Lopes Dec 6, 2024 •

edited

Loading

Chris-Hibbert Dec 6, 2024 •

edited

Loading

Jorge-Lopes commented Dec 5, 2024 •

edited

Loading

Jorge-Lopes commented Dec 5, 2024 •

edited

Loading

Chris-Hibbert commented Dec 5, 2024 •

edited

Loading

Jorge-Lopes commented Dec 10, 2024 •

edited

Loading

Jorge-Lopes Dec 11, 2024 •

edited

Loading